Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Silence ECONNRESET errors after connection close #392

Merged
merged 1 commit into from
Mar 21, 2022
Merged

fix: Silence ECONNRESET errors after connection close #392

merged 1 commit into from
Mar 21, 2022

Conversation

joeyparrish
Copy link
Contributor

See also less/less.js#3693

Fixes #391

joeyparrish added a commit to joeyparrish/less.js that referenced this pull request Mar 15, 2022
@joeyparrish
fix: Use pre-release fix for needle
aef9a5c 
See also:
 - shaka-project/shaka-player#3981
 - less#3693
 - tomas/needle#392
 - nodejs/node#27916
joeyparrish added a commit to joeyparrish/less.js that referenced this pull request Mar 15, 2022
@joeyparrish
fix: Use pre-release fix for needle
e7ac948 
See also:
 - shaka-project/shaka-player#3981
 - less#3693
 - tomas/needle#392
 - nodejs/node#27916
joeyparrish added a commit to joeyparrish/less.js that referenced this pull request Mar 15, 2022
@joeyparrish
fix: Use pre-release fix for needle
4fb5384 
See also:
 - shaka-project/shaka-player#3981
 - less#3693
 - tomas/needle#392
 - nodejs/node#27916
@tomas
Copy link
Owner

tomas commented Mar 15, 2022

Thanks! Are you able to run all tests successfully?

@joeyparrish
Copy link
Contributor Author

I get some test failures, but it's the same tests with and without my change:

  295 passing (10s)
  2 pending
  7 failing

  1) character encoding
       Given content-type: "text/html; charset=EUC-JP"
         with decode = false
           does not decode:
     Uncaught TypeError: Cannot read properties of undefined (reading 'body')
      at /home/joey/hacking/work/forks/needle/test/decoder_spec.js:23:16
      at done (lib/needle.js:493:14)
      at ClientRequest.had_error (lib/needle.js:508:5)
      at ClientRequest.emit (node:events:390:28)
      at Socket.socketErrorListener (node:_http_client:447:9)
      at Socket.emit (node:events:390:28)
      at emitErrorNT (node:internal/streams/destroy:157:8)
      at emitErrorCloseNT (node:internal/streams/destroy:122:3)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  2) character encoding
       Given content-type: "text/html; charset=EUC-JP"
         with decode = true
           decodes:
     Uncaught TypeError: Cannot read properties of undefined (reading 'body')
      at /home/joey/hacking/work/forks/needle/test/decoder_spec.js:38:16
      at done (lib/needle.js:493:14)
      at ClientRequest.had_error (lib/needle.js:508:5)
      at ClientRequest.emit (node:events:390:28)
      at Socket.socketErrorListener (node:_http_client:447:9)
      at Socket.emit (node:events:390:28)
      at emitErrorNT (node:internal/streams/destroy:157:8)
      at emitErrorCloseNT (node:internal/streams/destroy:122:3)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  3) request stream length
       no stream_length set
         returns 400 if Transfer-Encoding is set to a blank string:

      Uncaught AssertionError: expected 200 to equal 400
      + expected - actual

      -200
      +400
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at /home/joey/hacking/work/forks/needle/test/request_stream_spec.js:66:34
      at done (lib/needle.js:493:14)
      at PassThrough.<anonymous> (lib/needle.js:756:9)
      at PassThrough.emit (node:events:402:35)
      at endReadableNT (node:internal/streams/readable:1343:12)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  4) request stream length
       stream_length set to invalid value
         returns 400 if Transfer-Encoding is set to a blank string:

      Uncaught AssertionError: expected 200 to equal 400
      + expected - actual

      -200
      +400
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at /home/joey/hacking/work/forks/needle/test/request_stream_spec.js:96:34
      at done (lib/needle.js:493:14)
      at PassThrough.<anonymous> (lib/needle.js:756:9)
      at PassThrough.emit (node:events:402:35)
      at endReadableNT (node:internal/streams/readable:1343:12)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  5) request stream length
       stream_length set to invalid value
         returns 400 if Transfer-Encoding is not set:

      Uncaught AssertionError: expected 200 to equal 400
      + expected - actual

      -200
      +400
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at /home/joey/hacking/work/forks/needle/test/request_stream_spec.js:104:34
      at done (lib/needle.js:493:14)
      at PassThrough.<anonymous> (lib/needle.js:756:9)
      at PassThrough.emit (node:events:402:35)
      at endReadableNT (node:internal/streams/readable:1343:12)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  6) request stream length
       stream_length set to 0
         stream without path
           returns 400 if Transfer-Encoding is set to a blank string:

      Uncaught AssertionError: expected 200 to equal 400
      + expected - actual

      -200
      +400
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at /home/joey/hacking/work/forks/needle/test/request_stream_spec.js:213:36
      at done (lib/needle.js:493:14)
      at PassThrough.<anonymous> (lib/needle.js:756:9)
      at PassThrough.emit (node:events:402:35)
      at endReadableNT (node:internal/streams/readable:1343:12)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  7) socket cleanup
       should cleanup sockets on ERR_STREAM_PREMATURE_CLOSE (using stream.pipeline):

      Uncaught AssertionError: expected 1 to equal 0
      + expected - actual

      -1
      +0
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at Timeout._onTimeout (test/socket_cleanup_spec.js:73:33)
      at listOnTimeout (node:internal/timers:557:17)
      at processTimers (node:internal/timers:500:7)

@joeyparrish
Copy link
Contributor Author

@tomas, since the test failures I'm seeing do not appear to be regressions, can we merge this change? Less.js is waiting on this fix to update their dependency. Without this fix, less v4 is broken on macOS.

@tomas
Copy link
Owner

tomas commented Mar 21, 2022

Yes sure. Thanks for the heads up.

@tomas tomas merged commit 67ad8a4 into tomas:master Mar 21, 2022
@tomas
Copy link
Owner

tomas commented Mar 21, 2022 •
edited
Loading

Hold on, this is breaking the tests in socket_pool_spec.js: (Node v12.21.0)

image

@tomas
Copy link
Owner

tomas commented Mar 21, 2022

This seems to fix the issue, but I'm not sure if it solves #392.

image

This might also be relevant:

image

I don't remember exactly why I commented that part of the code, but it seems related.

@joeyparrish
Copy link
Contributor Author

I'll test on mac and see if the issue is still fixed with that change.

@joeyparrish
Copy link
Contributor Author

It looks like that change would reintroduce the ECONNRESET bug. request.socket.destroyed seems to always be false when that code is executed.

If you disable my fix, the bug I'm fixing seems to be triggered by one of the existing tests on macOS, FWIW:
./node_modules/.bin/mocha --grep EPIPE test

  1) when posting a very long string
       shouldn't throw an EPIPE error out of nowhere:
     Uncaught Error: read ECONNRESET
      at WriteWrap.onWriteComplete [as oncomplete] (node:internal/stream_base_commons:94:16)

I don't see any of the socket reuse tests failing, for some reasons, neither on macOS nor on Linux. But I see many test failures with or without my changes, and they are inconsistent and different across operating systems.

Does limiting my workaround to macOS solve the test failure for you? (Since I can't reproduce the failure, I can't say if this is helpful or not, but I'm assuming we have some OS-specific difference at play.)

if (process.platform == 'darwin') request.abort();

@joeyparrish joeyparrish mentioned this pull request Mar 22, 2022
Closed
@joeyparrish
Copy link
Contributor Author

I filed issues for the tests that fail on my mac and Linux workstations with node v16, and I uploaded PRs to fix them. I'll try node v12 on Linux via nvm and see if I can reproduce that reuse issue.

@joeyparrish joeyparrish deleted the fix-mac-econnreset branch March 22, 2022 01:17
@joeyparrish
Copy link
Contributor Author

Yup, there it is. Reverting this fixes it.

nvm exec 12 node node_modules/.bin/mocha test

@joeyparrish joeyparrish mentioned this pull request Mar 22, 2022
Closed
joeyparrish added a commit to joeyparrish/needle that referenced this pull request Mar 22, 2022
@joeyparrish
fix: Fix test failures in node v4-v12
2174b32 
These failures were introduced by PR tomas#392

Closes tomas#397
joeyparrish added a commit to joeyparrish/needle that referenced this pull request Mar 22, 2022
@joeyparrish
fix: Fix test failures in node v4-v12
64dfba1 
These failures were introduced by PR tomas#392

Rather than abort the request, set up a listener to silence any errors
thrown late, as happens on macOS in certain node versions.

Closes tomas#397
@joeyparrish joeyparrish mentioned this pull request Mar 22, 2022
Merged
@joeyparrish
Copy link
Contributor Author

I have PRs up to fix all the various test failures I've seen, including the reuse issue you found. With those, I have tests passing on Linux and macOS on node v4, v6, v8, v10, v12, v14, v16, and v17.

joeyparrish added a commit to joeyparrish/needle that referenced this pull request Mar 22, 2022
@joeyparrish
fix: Fix test failures in node v4-v12
721ef83 
These failures were introduced by PR tomas#392

Rather than abort the request, set up a listener to silence any errors
thrown late, as happens on macOS in certain node versions.

Closes tomas#397
joeyparrish added a commit to joeyparrish/less.js that referenced this pull request Apr 20, 2022
@joeyparrish
fix: Fix https failures on macOS
3608a27 
The issue was traced upstream to needle, and resolved in:
 - tomas/needle#392
 - tomas/needle#394
 - tomas/needle#396
 - tomas/needle#398

Closes less#3693
@joeyparrish joeyparrish mentioned this pull request Apr 20, 2022
Merged
3 tasks
iChenLei pushed a commit to less/less.js that referenced this pull request Apr 22, 2022
@joeyparrish
fix: Fix https failures on macOS (#3716)
1eafc06 
The issue was traced upstream to needle, and resolved in:
 - tomas/needle#392
 - tomas/needle#394
 - tomas/needle#396
 - tomas/needle#398

Closes #3693
lumburr pushed a commit to lumburr/less.js that referenced this pull request Apr 28, 2022
@joeyparrish @lumburr
fix: Fix https failures on macOS (less#3716)
97f7082 
The issue was traced upstream to needle, and resolved in:
 - tomas/needle#392
 - tomas/needle#394
 - tomas/needle#396
 - tomas/needle#398

Closes less#3693
@vAHiD55555 vAHiD55555 mentioned this pull request Jun 19, 2024
Open
@ruhi47 ruhi47 mentioned this pull request Aug 21, 2024
Closed
@BIGboss248 BIGboss248 mentioned this pull request Aug 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ECONNRESET thrown after connection closed
2 participants
@joeyparrish @tomas